Repository navigation
fix(schemas): align inline submitted branches with their published async status files - #8934
sujanchalla0510 wants to merge 5 commits into
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated paths touched without an approving review.
This PR modifies seven schema files under static/schemas/source/**, which fall under the repo's hard, path-based approval gate (gated_paths: true). The current review_decision is REVIEW_REQUIRED, not APPROVED, so per decision-table row 2 the outcome must be escalate — human/CODEOWNERS review is required before merge.
The reviewer reported a clean patch with no findings: the change removes not.anyOf clauses from the inline submitted oneOf branches of seven response schemas to align them with the published async-submitted envelopes, cited as a DR-0036 correction with a patch changeset. The loosening is verified non-breaking, oneOf classification is unchanged, and no docs drift was found. So no blocking (critical/high) findings exist — but the gated-paths rule is deterministic and independent of code quality.
Escalation reasons
- Modifies gated schema files under
static/schemas/source/**whilereview_decisionisREVIEW_REQUIRED(notAPPROVED) — human/CODEOWNERS approval required. Affected files includesync-creatives-response.json,build-creative-response.json,control-media-buy-response.json,create-media-buy-response.json,media-buy-commitment-response.json,sync-catalogs-response.json, andupdate-media-buy-response.json.
Why human review
- Modifies gated paths under static/schemas/source/** (7 response schemas) while review_decision is REVIEW_REQUIRED (not APPROVED) — human/CODEOWNERS approval required (decision rule row 2).
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/creative/sync-creatives-response.json (modified) matches
static/schemas/source/**; static/schemas/source/media-buy/build-creative-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/control-media-buy-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/create-media-buy-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/media-buy-commitment-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/sync-catalogs-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/update-media-buy-response.json (modified) matchesstatic/schemas/source/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
aao-secretariat
Bot
added
the
ladon/needs-human-review
KonstantinMirin
left a comment
KonstantinMirin
left a comment
There was a problem hiding this comment.
Thanks for taking this on. Each inline submitted branch now accepts what its published status file accepts. I found one side effect at the union level: on three tools, a body can now match two branches of the response oneOf, so it fails where it passed before.
On create_media_buy, update_media_buy and build_creative, the success branches don't exclude a submitted body. Now that the submitted branch no longer excludes the result fields, a body that carries both matches both branches:
| Body | Base | This PR |
|---|---|---|
create_media_buy: status: "submitted", task_id, media_buy_id, confirmed_at, revision, packages |
valid (Success) | invalid (Success and Submitted) |
update_media_buy: status: "submitted", task_id, media_buy_id, revision |
valid (Success) | invalid (both) |
build_creative: status: "submitted", task_id, creative_manifest (also creative_manifests) |
valid (Success) | invalid (both) |
So "every value the inline branch accepted before still validates" holds for each branch, but not for the response schema a validator runs. The other four are fine. The sync_creatives success branch already excludes task_id and status: "submitted". control_media_buy, the commitment response and sync_catalogs fix success to status: "completed".
A possible fix: add { "$ref": "/schemas/core/task-submitted.json" } to the not of each success branch on those three tools, which is all four success branches on build_creative. A body that satisfies core/task-submitted.json then matches Submitted only, and a body without task_id still matches Success as before. With that change, all three rows validate as Submitted, and the same bodies with status: "completed" still validate as Success. I checked this with ajv against the base, this head, and this head with the change.
…ive success branches KonstantinMirin's review of adcontextprotocol#8934: loosening the inline submitted branches let a submitted body carrying result fields match both the success and submitted branches of create_media_buy, update_media_buy and build_creative, failing the response oneOf. Add core/task-submitted.json to the not of each success branch (all four on build_creative), so such bodies match Submitted only.
…oneOf exclusivity, adcontextprotocol#8934 review)
@KonstantinMirin — fixed in
Verified with ajv (draft-07, refs resolved against
Local On the CI Canonical tests (2/4) failure that predates your review on 120e172: still being tracked — the shard-2 failing stage is not visible from the contributor side (job logs need upstream admin), so if you can see which stage fails there, that would unblock it. |
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated paths require human/CODEOWNERS approval.
This PR modifies 7 response schemas under static/schemas/source/**, which are under a hard, deterministic approval gate (gated_paths: true). The current review_decision is REVIEW_REQUIRED, not APPROVED, so decision rule row 2 fires: the gate is not satisfied and the PR cannot be auto-approved regardless of how clean the diff is.
The reviewer pass was clean — the delta adds not: {$ref: task-submitted} to success branches (directly or folded into existing not.anyOf exclusions), pairing with removal of submitted-branch result-field guards. oneOf exclusivity was verified to hold, the loosening aligns inline with the published status file (DR-0008/DR-0036), is relaxation-only / non-breaking, a changeset is present, and there is no schema↔docs drift. No Critical/High/Medium findings.
Despite the clean review, the hard gate stands until a human/CODEOWNERS approval is recorded. This also matches the prior decision (escalate), which persists here because the gated-paths condition remains unsatisfied.
Escalation reasons
- Modifies files under the hard approval gate
static/schemas/source/**(7 response schemas) whilereview_decisionisREVIEW_REQUIRED(notAPPROVED). Human/CODEOWNERS approval is required before merge.
Why human review
- Modifies gated paths under static/schemas/source/** (7 response schemas: sync-creatives-response, build-creative-response, control-media-buy-response, create-media-buy-response, media-buy-commitment-response, sync-catalogs-response, update-media-buy-response) while review_decision is REVIEW_REQUIRED (not APPROVED) — human/CODEOWNERS approval required (decision rule row 2).
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/creative/sync-creatives-response.json (modified) matches
static/schemas/source/**; static/schemas/source/media-buy/build-creative-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/control-media-buy-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/create-media-buy-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/media-buy-commitment-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/sync-catalogs-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/update-media-buy-response.json (modified) matchesstatic/schemas/source/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated-path hard gate not satisfied.
This PR modifies 7 schema files under static/schemas/source/**, all of which are under a hard, non-overridable approval gate (gated_paths: true). The current review_decision is REVIEW_REQUIRED, not APPROVED, so decision rule row 2 forces escalation regardless of how clean the diff is.
The reviewer found no Critical/High/Medium findings. The oneOf disjointness refactor was verified: submitted branches drop not.anyOf to align with published *-async-response-submitted.json shapes, success branches gain not: {$ref: core/task-submitted.json} so discrimination moves cleanly from field-presence to the status: "submitted" const. Changeset present and correctly scoped (patch under DR-0036); schema↔docs coherent. This matches the prior-run finding set (none).
Although there are no new actionable findings (so sticky escalation per row 6 would not apply), the gated-paths gate (row 2) stands independently and requires a human/CODEOWNERS approval before merge.
Escalation reasons:
- Modifies gated paths under
static/schemas/source/**(7 response schemas) whilereview_decisionisREVIEW_REQUIRED, notAPPROVED— human/CODEOWNERS approval required (decision rule row 2). Gate lifts automatically once an APPROVED review is recorded.
Why human review
- Modifies gated paths under static/schemas/source/** (7 response schemas: sync-creatives-response, build-creative-response, control-media-buy-response, create-media-buy-response, media-buy-commitment-response, sync-catalogs-response, update-media-buy-response) while review_decision is REVIEW_REQUIRED (not APPROVED) — human/CODEOWNERS approval required (decision rule row 2).
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/creative/sync-creatives-response.json (modified) matches
static/schemas/source/**; static/schemas/source/media-buy/build-creative-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/control-media-buy-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/create-media-buy-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/media-buy-commitment-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/sync-catalogs-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/update-media-buy-response.json (modified) matchesstatic/schemas/source/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
…ocol#8856 alignment Canonical tests (2/4) failed on this PR: the relationship-scoped indicators test pinned submitted+warnings as invalid for create, update, commitment, control and sync — the exact behavior adcontextprotocol#8856 corrects, since the published *-async-response-submitted.json files accept warnings on the submitted envelope (the issue names warnings for control_media_buy). Flip the five submitted-arm assertions to valid, with a comment citing adcontextprotocol#8856/DR-0036; error-arm and sync-success exclusions unchanged. 18/18 pass locally.
Update in Shard 2/4 is every fourth stage of the root test script; the failing stage is Fix: the five submitted-arm assertions now expect valid, with a comment citing #8856/DR-0036; the test title is adjusted to match. Error-arm exclusions and the sync_creatives success-arm exclusion are unchanged. The test passes 18/18 locally against schemas built from this branch, and the changeset notes the test change. @KonstantinMirin — flagging this one for your eye in particular, since the alternative (keeping |
KonstantinMirin
left a comment
KonstantinMirin
left a comment
There was a problem hiding this comment.
@sujanchalla0510 Please keep decfeb26. Allowing warnings on the submitted branches is right.
The published -async-response-submitted.json files are open and don't name warnings, so a submitted body that carries it already validates against them. Forbidding it only in the inline branch is the mismatch #8856 reports. Forbidding it in the published files too would narrow them within 3.x.
The meaning doesn't change. warnings is defined only on the success branches, where "the media buy was still created". On a submitted envelope it's a field the schema doesn't define, like any other extra field. An advisory known at submit time already has a defined place: the envelope's errors, "for non-blocking warnings".
I checked decfeb26 with ajv. Submitted plus warnings validates against the inline and published schemas for create, update and control. A submitted body with media_buy_id and packages matches only the Submitted branch.
Analyzed and generated with the help of Claude, fully reviewed by me.
Fixes #8856.
Problem
Eight tasks defined their
submittedresponse twice — as an inline branch in<tool>-response.jsonand as a published<tool>-async-response-submitted.json— and the copies disagreed: every inline branch carried anotforbidding result fields that the published status file accepts (both are open schemas withadditionalProperties: trueand nonot), so the same response validated differently depending on which schema a validator used. Example from the issue:control_media_buy's inline submitted branch rejectedmedia_buy_idandwarnings; its published file accepts them.Enumeration
I compared every inline status branch against its published async file (properties, required, and forbidden fields, following
$ref/allOf). Noworkingorinput-requiredinline branches exist anywhere — onlycore/task-working.json/core/task-input-required.json. The disagreeing submitted branches:get_creative_featuresis the control case: its inline submittednotexactly mirrors its published file'snot, so it is unchanged.Change
Remove the
notfrom those seven inline submitted branches (7 files), so each inline branch accepts exactly what its published status file accepts. Each fix is a recorded correction under DR-0036 — the published file already states the shape — and a patch changeset records it. Nothing else changes: the completed/failed branches keep theirnots (no published status file states a different shape for them), and there is no restructuring — the bodies-only layout and generated unions stay in #8565 for 4.0, and the out-of-scopefailed-requires-errorsnarrowing is untouched.Testing
{status: submitted, task_id, media_buy_id}) FAILED the inline response schema and PASSED the published async file, on all eight tasks.failedcontrol_media_buy payload carryingmedia_buy_idstill fails (error-branchnotuntouched), and a submitted payload withouttask_idstill fails.node tests/schema-validation.test.cjs: 35/36 pass. The one failure is environmental in my sparse checkout — it cannot loaddist/schemas/3.2.1/core/opportunity-context.json(released dist trees are not in the sparse checkout) and is unrelated to this diff; CI runs it on a full tree.🤖 Generated with Claude Code